Cloud netconfig #19922

pdostal · 2024-08-09T18:40:06Z

There are 2 commits, first is just cleanup and second is improvements.
The new terraform file is just old azure.tf with two network interfaces each with primary public IP address and secondary private IP address.

This is not yet finish. I'll add more test and I'm not happy with the Terraform part. Similar testing should be also done in EC2 and GCE and I'd like to do it in one pull request as it won't differ much - only Terraform part will differ a lot.

If you have suggestion I'd appreciate those.

Follows: Add test for cloud-netconfig #18095
Related ticket: poo#136235
Verification run:
Build20240812-1 of sle-15-SP6-Azure-Basic-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 45 minutes ago
Build20240812-1 of sle-15-SP6-EC2-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 about an hour ago
Build20240812-1 of sle-15-SP6-GCE-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 about an hour ago

tests/publiccloud/cloud_netconfig.pm

asmorodskyi

LGTM

grisu48

Overall nice work. I left some improvement comments but overall this should work.

I'm not sure if we need an entirely new terraform profile, given the similarities with azure.tf 🤔

grisu48 · 2024-08-19T11:58:31Z

tests/publiccloud/cloud_netconfig.pm

-        cmd => 'systemctl status cloud-netconfig.timer',
-        fail_message => 'cloud-netconfig.timer appears not to be started.'
-    );
+    $instance->ssh_assert_script_run('systemctl status cloud-netconfig.timer');


Suggested change

$instance->ssh_assert_script_run('systemctl status cloud-netconfig.timer');

$instance->ssh_assert_script_run('systemctl is-active cloud-netconfig.timer');

grisu48 · 2024-08-19T12:02:23Z

tests/publiccloud/cloud_netconfig.pm

    chomp($local_eth0_ip);
+    my $metadata_eth0_ip = $provider->query_metadata($instance, ifNum => '0', addrCount => '0');
+    die("Failed to get interface 0 IP from metadata server") unless length($metadata_eth0_ip);


Suggested change

die("Failed to get interface 0 IP from metadata server") unless length($metadata_eth0_ip);

die("Failed to get interface IPs from metadata server") unless length($metadata_eth0_ip);

I would remove the interface numbers here, as they are confusing to anyone who has not the full code in their head. I think this makes a better error message, because it still explains what happens but without unnecessary (and confusing) interface indices.

I applied your suggestion and moved the check inside $provider->query_metadata().

grisu48 · 2024-08-19T12:03:35Z

tests/publiccloud/cloud_netconfig.pm

-    } else {
-        die("Unsupported public cloud provider.");
+    if (is_azure) {
+        record_info('eth1', 'Get public IP address for eth1');


I think this information would be better as a code comment instead of a information box.

grisu48 · 2024-08-19T12:04:06Z

tests/publiccloud/cloud_netconfig.pm

-        die("Unsupported public cloud provider.");
+    if (is_azure) {
+        record_info('eth1', 'Get public IP address for eth1');
+        my $local_eth1_ip = $instance->ssh_script_output(qq(ip -4 -o a s eth1 primary | grep -Po "inet \\K[\\d.]+" | head -n1));


Why is head -n1 necessary here? 🤔

grisu48 · 2024-08-19T12:04:22Z

tests/publiccloud/cloud_netconfig.pm

+        my $local_eth1_ip = $instance->ssh_script_output(qq(ip -4 -o a s eth1 primary | grep -Po "inet \\K[\\d.]+" | head -n1));
+        chomp($local_eth1_ip);
+        my $metadata_eth1_ip = $provider->query_metadata($instance, ifNum => '1', addrCount => '0');
+        die("Failed to get interface 1 IP from metadata server") unless length($metadata_eth1_ip);


Suggested change

die("Failed to get interface 1 IP from metadata server") unless length($metadata_eth1_ip);

die("Failed to get interface IP from metadata server") unless length($metadata_eth1_ip);

I applied your suggestion and moved the check inside $provider->query_metadata().

grisu48 · 2024-08-19T12:12:10Z

tests/publiccloud/cloud_netconfig.pm

+        die('There are no ARP neighbors on eth0') if ($instance->ssh_script_output("ip neighbor show dev eth0 | wc -l") == 0);
+        die('There are no ARP neighbors on eth1') if ($instance->ssh_script_output("ip neighbor show dev eth1 | wc -l") == 0);
+
+        record_info('Local removal', 'Remove eth0 secondary address and eth1 default route and check if it reappears.');


I don't see the point of this check for cloud_netconfig. This is testing something different IMHO and could be removed. What do you think? 🤔

From https://github.com/SUSE-Enceladus/cloud-netconfig/blob/master/README.md:

Interface configurations will be checked periodically on each DHCP lease renewal and additionally, if the systemd timer is enabled, every 60 seconds. cloud-netconfig detects changes in the metadata configuration and updates interface configurations and routing policies accordingly. This means that IP addresses and ranges that were removed from the virtual interface configuration will be removed from the interface, but only addresses and ranges that were automatically added by cloud-netconfig will be removed. Addresses added manually by the administrator or by another tool (e.g. high-availability software) will not be touched.

From my understanding cloud-netconfig periodically checks if the interfaces and addresses defined in cloud metadata are present in the system. This checks that by removing some and checking if they got reapplied.

I see, yes then it makes sense 👍

grisu48 · 2024-08-19T12:12:21Z

tests/publiccloud/cloud_netconfig.pm

+        $instance->ssh_assert_script_run("sudo ip route del default dev eth1");
+        record_info('debug', $instance->ssh_script_output('ip addr show; ip route show'));
+
+        sleep(90);


I replaced it with systemctl start cloud-netconfig.service so please recheck

grisu48 · 2024-08-19T12:13:22Z

tests/publiccloud/cloud_netconfig.pm

+        my $nic_name = script_output(qq(az network nic list --resource-group $resource_group | jq -r '.[]|select(.primary==false).name'));
+        my $ipConfig_name = script_output(qq(az network nic list --resource-group $resource_group | jq -r '.[]|select(.primary==false).ipConfigurations[]|select(.privateIPAddress=="$local_eth1_secondary_ip").name'));
+        assert_script_run("az network nic ip-config delete -g $resource_group -n $ipConfig_name --nic-name $nic_name");
+        sleep(90);


Please add a comment explaining why the sleep is needed.

I replaced it with systemctl start cloud-netconfig.service so please recheck

grisu48 · 2024-08-19T12:14:01Z

tests/publiccloud/cloud_netconfig.pm

+        assert_script_run("az network nic ip-config delete -g $resource_group -n $ipConfig_name --nic-name $nic_name");
+        sleep(90);
+        record_info('debug', $instance->ssh_script_output('ip addr show; ip route show'));
+        die('There is secondary IP address in eth1 altho it has been removed in the provider') if ($instance->ssh_script_run(qq(ip -4 -o a s dev eth1 secondary | grep -Po "inet \\K[\\d.]+" | wc -l)) != 0);


Suggested change

die('There is secondary IP address in eth1 altho it has been removed in the provider') if ($instance->ssh_script_run(qq(ip -4 -o a s dev eth1 secondary | grep -Po "inet \\K[\\d.]+" | wc -l)) != 0);

die('Secondary IP address in eth1 still present after removed via provider') if ($instance->ssh_script_run(qq(ip -4 -o a s dev eth1 secondary | grep -Po "inet \\K[\\d.]+" | wc -l)) != 0);

grisu48 · 2024-08-19T12:15:30Z

tests/publiccloud/cloud_netconfig.pm

+    my $instance = shift;
+    record_info('DEBUG');
+
+    $instance->ssh_script_run("ip -j a s");


Necessary if the following commands print exactly the same (just not as json)? 🤔

pdostal · 2024-08-21T12:48:48Z

Build20240820-1 of sle-15-SP6-Azure-BYOS-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 17 minutes ago
Build20240820-1 of sle-15-SP6-EC2-BYOS-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 22 minutes ago
Build20240820-1 of sle-15-SP6-EC2-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 22 minutes ago
Build20240820-1 of sle-15-SP6-GCE-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 23 minutes ago
Build20240820-1 of sle-15-SP6-GCE-BYOS-Updates.aarch64 publiccloud_cloud_netconfig@64bit 4 23 minutes ago
Build20240820-1 of sle-12-SP5-Azure-BYOS-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 26 minutes ago
Build20240820-1 of sle-15-SP6-Azure-Standard-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 29 minutes ago
Build20240820-1 of sle-15-SP6-Azure-Basic-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 30 minutes ago
Build20240820-1 of sle-15-SP6-Azure-BYOS-Updates.x86_64 publiccloud_cloud_netconfig@64bit 4 32 minutes ago

asmorodskyi · 2024-08-21T16:21:02Z

data/publiccloud/terraform/azure_cloud-netconfig.tf

+  default = "gen2"
+}
+
+variable "extra-disk-size" {


if we creating dedicated profile we may cleaned up from things which are not needed for this certain test . we won't add extra disk here so let's drop quite some code from it

asmorodskyi · 2024-08-21T16:21:55Z

data/publiccloud/terraform/azure_cloud-netconfig.tf

+  default = false
+}
+
+variable "cloud_init" {


same here this variable is needed by another test and if it is test suite specific profile we can drop this

asmorodskyi

last comments from me is just nice to have , feel free to ignore them . LGTM

pdostal requested review from grisu48, asmorodskyi, ricardobranco777 and m-dati as code owners August 9, 2024 18:40

pdostal added the WIP Work in progress label Aug 9, 2024

pdostal force-pushed the cloud_netconfig branch from cfc69a8 to 0b0041f Compare August 9, 2024 18:45

asmorodskyi reviewed Aug 9, 2024

View reviewed changes

tests/publiccloud/cloud_netconfig.pm Outdated Show resolved Hide resolved

asmorodskyi reviewed Aug 9, 2024

View reviewed changes

tests/publiccloud/cloud_netconfig.pm Outdated Show resolved Hide resolved

asmorodskyi reviewed Aug 9, 2024

View reviewed changes

tests/publiccloud/cloud_netconfig.pm Show resolved Hide resolved

pdostal force-pushed the cloud_netconfig branch from 0b0041f to 74dcb9f Compare August 12, 2024 12:10

pdostal requested a review from mpagot as a code owner August 12, 2024 12:10

pdostal force-pushed the cloud_netconfig branch from 74dcb9f to 56f1d5a Compare August 12, 2024 15:12

ricardobranco777 approved these changes Aug 12, 2024

View reviewed changes

pdostal force-pushed the cloud_netconfig branch from 56f1d5a to 794da82 Compare August 13, 2024 12:19

pdostal removed the WIP Work in progress label Aug 13, 2024

asmorodskyi approved these changes Aug 16, 2024

View reviewed changes

grisu48 reviewed Aug 19, 2024

View reviewed changes

Simplify publiccloud/cloud_netconfig test

7bcf20c

pdostal force-pushed the cloud_netconfig branch 2 times, most recently from 762d973 to 0757bc7 Compare August 21, 2024 12:48

asmorodskyi reviewed Aug 21, 2024

View reviewed changes

asmorodskyi approved these changes Aug 21, 2024

View reviewed changes

Test Azure cloud-netconfig addressing

650be00

pdostal force-pushed the cloud_netconfig branch from 0757bc7 to 650be00 Compare August 22, 2024 07:37

asmorodskyi merged commit 4251800 into os-autoinst:master Aug 22, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cloud netconfig #19922

Cloud netconfig #19922

pdostal commented Aug 9, 2024 •

edited

Loading

asmorodskyi left a comment

grisu48 left a comment

grisu48 Aug 19, 2024

grisu48 Aug 19, 2024

pdostal Aug 21, 2024

grisu48 Aug 19, 2024

grisu48 Aug 19, 2024

grisu48 Aug 19, 2024

pdostal Aug 21, 2024

grisu48 Aug 19, 2024

pdostal Aug 19, 2024

grisu48 Aug 19, 2024

grisu48 Aug 19, 2024

pdostal Aug 21, 2024

grisu48 Aug 19, 2024

pdostal Aug 21, 2024

grisu48 Aug 19, 2024

grisu48 Aug 19, 2024

pdostal commented Aug 21, 2024

asmorodskyi Aug 21, 2024

asmorodskyi Aug 21, 2024

asmorodskyi left a comment

	$instance->ssh_assert_script_run('systemctl status cloud-netconfig.timer');
	$instance->ssh_assert_script_run('systemctl is-active cloud-netconfig.timer');

	die("Failed to get interface 0 IP from metadata server") unless length($metadata_eth0_ip);
	die("Failed to get interface IPs from metadata server") unless length($metadata_eth0_ip);

	die("Failed to get interface 1 IP from metadata server") unless length($metadata_eth1_ip);
	die("Failed to get interface IP from metadata server") unless length($metadata_eth1_ip);

	die('There is secondary IP address in eth1 altho it has been removed in the provider') if ($instance->ssh_script_run(qq(ip -4 -o a s dev eth1 secondary \| grep -Po "inet \\K[\\d.]+" \| wc -l)) != 0);
	die('Secondary IP address in eth1 still present after removed via provider') if ($instance->ssh_script_run(qq(ip -4 -o a s dev eth1 secondary \| grep -Po "inet \\K[\\d.]+" \| wc -l)) != 0);

Cloud netconfig #19922

Cloud netconfig #19922

Conversation

pdostal commented Aug 9, 2024 • edited Loading

asmorodskyi left a comment

Choose a reason for hiding this comment

grisu48 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdostal commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmorodskyi left a comment

Choose a reason for hiding this comment

pdostal commented Aug 9, 2024 •

edited

Loading